feat(kuma-cp): deduplicate dataplane inbounds by address and port combination #12760

Automaat · 2025-02-05T14:37:22Z

Motivation

We now can have multiple inbounds if multiple services select the same address and port, but we will generate Envoy resources only for one of these. We should deduplicate them on creation so that we don't have inbounds that don't correlate with Envoy resources. This can be also useful for GUI

xref #13108

Fix: #12123

Signed-off-by: Marcin Skalski <[email protected]>

github-actions · 2025-02-05T14:45:43Z

Reviewer Checklist

🔍 Each of these sections need to be checked by the reviewer of the PR 🔍:
If something doesn't apply please check the box and add a justification if the reason is non obvious.

Is the PR title satisfactory? Is this part of a larger feature and should be grouped using > Changelog?
PR description is clear and complete. It Links to relevant issue as well as docs and UI issues
This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as an image registry)
IPv6 is taken into account (.e.g: no string concatenation of host port)
Tests (Unit test, E2E tests, manual test on universal and k8s)
- Don't forget ci/ labels to run additional/fewer tests
Does this contain a change that needs to be notified to users? In this case, UPGRADE.md should be updated.
Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label)

Signed-off-by: Marcin Skalski <[email protected]>

…rom-pod # Conflicts: # pkg/plugins/runtime/k8s/controllers/pod_controller_test.go # pkg/plugins/runtime/k8s/controllers/testdata/01.dataplane.yaml

Signed-off-by: Marcin Skalski <[email protected]>

jijiechen · 2025-02-24T07:53:12Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

+		ifaces = append(ifaces, inboundForServiceless(zone, pod, name, nodeLabels))
+	}
+
+	// Right now we will build multiple inbounds for each service selecting port, but later on


The code will be left here for future usage, so readers are from the future. Should it be changed to "We are only create one listener for last inbound now, but in the previous version, LegacyInboundInterfacesFor, we built multiple inbounds for each service selecting port"

jijiechen

Looks good to me.

not ready yet

Signed-off-by: Marcin Skalski <[email protected]>

pkg/plugins/runtime/k8s/controllers/pod_controller.go

jijiechen · 2025-03-21T08:48:45Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

@@ -166,6 +168,7 @@ func (i *InboundConverter) LegacyInboundInterfacesFor(ctx context.Context, zone
 	return ifaces, nil
 }

+// Should be used when MeshService mode is Exclusive
 func (i *InboundConverter) InboundInterfacesFor(ctx context.Context, zone string, pod *kube_core.Pod, services []*kube_core.Service) ([]*mesh_proto.Dataplane_Networking_Inbound, error) {


Do we need to sort services []*kube_core.Service before iterate over it? To keep consistant results when deduplicated in multiple runs.

we sort inbounds at the end

jijiechen · 2025-03-21T08:59:49Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

+}
+
+func deduplicateInboundsByAddressAndPort(ifaces []*mesh_proto.Dataplane_Networking_Inbound) []*mesh_proto.Dataplane_Networking_Inbound {
+	inboundKey := func(iface *mesh_proto.Dataplane_Networking_Inbound) string {


In one of my testing, I found when the service port is different than the targetPort, the two inbounds generated on the DP has the same name and port, but they have different k8s.kuma.io/service-port tags: one of them is incorrect. So in this case, we need to remove the one with incorrect tag. Could you also verify this?

do you mean the situation when we have service with two ports?
With this approach we don't care about tags, inbound tags will be gone after we fully switch to MeshService exclusive. I think in this situation we will just create inbound from pod port

Signed-off-by: Marcin Skalski <[email protected]>

lobkovilya · 2025-03-24T10:49:17Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

+// to not change order of inbounds.
+// For gateway we pick first inbound to take tags from. Delegated gateway identity relies on this.
+// For Dataplanes when MeshService is disabled we base identity and routing
+// TODO: We should revisit this when we rework identity


you can reference the issue #3339

lobkovilya · 2025-03-24T10:50:07Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

 	return ifaces, nil
 }

+// Should be used when MeshService mode is Exclusive


nit: it's a public method, please use a proper doc comments that starts with // InboundInterfacesFor ...

updated comment

lobkovilya · 2025-03-24T10:58:03Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

 	return ifaces, nil
 }

+// Should be used when MeshService mode is Exclusive
+func (i *InboundConverter) InboundInterfacesFor(ctx context.Context, zone string, pod *kube_core.Pod, services []*kube_core.Service) ([]*mesh_proto.Dataplane_Networking_Inbound, error) {


Please create a private method inboundInterfaceFor so that methods looks like

func (i *InboundConverter) LegacyInboundInterfacesFor(ctx context.Context, zone string, pod *kube_core.Pod, services []*kube_core.Service) ([]*mesh_proto.Dataplane_Networking_Inbound, error) { return i.inboundInterfacesFor(ctx, zone, pod, services) } func (i *InboundConverter) InboundInterfacesFor(ctx context.Context, zone string, pod *kube_core.Pod, services []*kube_core.Service) ([]*mesh_proto.Dataplane_Networking_Inbound, error) { inbounds, err := i.inboundInterfacesFor(ctx, zone, pod, services) if err != nil { return err } return deduplicateInboundsByAddressAndPort(ifaces), nil }

Because in the future if duplicated code in LegacyInboundInterfacesFor and InboundInterfacesFor diverges it'll be hard to understand what it takes to remove LegacyInboundInterfacesFor

good idea, chaged this

lobkovilya · 2025-03-24T11:02:31Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

+	for _, v := range inboundsPerName {
+		deduplicatedInbounds = append(deduplicatedInbounds, v)
+	}
+	sort.Slice(deduplicatedInbounds, func(i, j int) bool {


Probably should be moved out from deduplicateInbounds method, "deduplicate" usually implies:

the complexity is O(N)

the order of the input slice is preserved in the output slice

removed sort

lobkovilya · 2025-03-24T11:05:57Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

+		return fmt.Sprintf("%s:%d", iface.Address, iface.Port)
+	}
+
+	inboundsPerName := map[string]*mesh_proto.Dataplane_Networking_Inbound{}


nit: inboundsPerAddressPort?

lobkovilya · 2025-03-24T11:08:29Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

+	inboundsPerName := map[string]*mesh_proto.Dataplane_Networking_Inbound{}
+	for _, iface := range ifaces {
+		if inboundsPerName[inboundKey(iface)] == nil {
+			inboundsPerName[inboundKey(iface)] = iface


append to the result slice here, that way you preserve the original order and you won't need another loop on inboundsPerName

… in upgrade md Signed-off-by: Marcin Skalski <[email protected]>

lobkovilya · 2025-03-28T10:22:34Z

pkg/plugins/runtime/k8s/controllers/inbound_converter.go

+		return inbounds, err
+	}
+
+	// Right now we will build multiple inbounds for each service selecting port, but later on


I'm a bit lost with "right now" and "later on", by "later" you mean like "later when Envoy receives the config"? Or "later" when we develop some other feature?

I agree. The comment will be persist here for long, so we should serve a future reader's perspective.

lobkovilya · 2025-03-28T10:42:41Z

pkg/plugins/runtime/k8s/controllers/pod_converter.go

+		var ifaces []*mesh_proto.Dataplane_Networking_Inbound
+		var err error
+		// deduplicate inbounds only when MeshService mode is exclusive. Without MeshServices we need duplicated inbounds
+		// for identity and traffic routing


how identity is affected? it's still based on kuma.io/service even with MeshServices no?

feat(kuma-cp): inbound name should be taken from pod instead of service

Loading
Loading status checks…

30d0259

Signed-off-by: Marcin Skalski <[email protected]>

Automaat added the ci/run-full-matrix label Feb 5, 2025

Automaat requested a review from a team as a code owner February 5, 2025 14:37

Automaat requested review from jijiechen and Icarus9913 February 5, 2025 14:37

Automaat added 2 commits February 6, 2025 15:18

feat(kuma-cp): fix tests

Loading
Loading status checks…

fc93dba

Signed-off-by: Marcin Skalski <[email protected]>

feat(kuma-cp): fix tests

Loading
Loading status checks…

514fe96

Signed-off-by: Marcin Skalski <[email protected]>

Automaat mentioned this pull request Feb 7, 2025

feat(kuma-cp): take inbound name from pod instead of service #12783

Merged

Automaat added 2 commits February 12, 2025 09:16

Merge remote-tracking branch 'origin/master' into feat/inbound-name-f…

Loading
Loading status checks…

dfe4edb

…rom-pod # Conflicts: # pkg/plugins/runtime/k8s/controllers/pod_controller_test.go # pkg/plugins/runtime/k8s/controllers/testdata/01.dataplane.yaml

fix(e2e): fix picking right interface for delegated gateway

Loading
Loading status checks…

8817d6d

Signed-off-by: Marcin Skalski <[email protected]>

Automaat changed the title ~~feat(kuma-cp): inbound name should be taken from pod instead of service~~ feat(kuma-cp): deduplicate dataplane inbounds by address and port conmbination Feb 12, 2025

Automaat changed the title ~~feat(kuma-cp): deduplicate dataplane inbounds by address and port conmbination~~ feat(kuma-cp): deduplicate dataplane inbounds by address and port combination Feb 12, 2025

Automaat added 2 commits February 12, 2025 13:56

fix(e2e): fix delegated gateway behaviour

Loading
Loading status checks…

aca4d0a

Signed-off-by: Marcin Skalski <[email protected]>

fix(e2e): keep first inbound when deduplicating

Loading
Loading status checks…

dab5391

Signed-off-by: Marcin Skalski <[email protected]>

jijiechen reviewed Feb 24, 2025

View reviewed changes

jijiechen previously approved these changes Feb 24, 2025

View reviewed changes

Automaat marked this pull request as draft March 17, 2025 10:28

Automaat and others added 7 commits March 17, 2025 11:45

Merge branch 'master' into feat/inbound-name-from-pod

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

Loading
Loading status checks…

b020271

Merge branch 'master' into feat/inbound-name-from-pod

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

Loading
Loading status checks…

d3a5a65

feat(inbounds): fix unit tests

Loading
Loading status checks…

3ad8235

Signed-off-by: Marcin Skalski <[email protected]>

feat(inbounds): deduplicate inbounds only for MeshService exclusive

Loading
Loading status checks…

653f1e6

Signed-off-by: Marcin Skalski <[email protected]>

feat(inbounds): add test for deduplication

Loading
Loading status checks…

21f1407

Signed-off-by: Marcin Skalski <[email protected]>

feat(inbounds): fix check

Loading
Loading status checks…

bb69589

Signed-off-by: Marcin Skalski <[email protected]>

feat(inbounds): update comments

Loading
Loading status checks…

2972853

Signed-off-by: Marcin Skalski <[email protected]>

Automaat marked this pull request as ready for review March 21, 2025 06:57

jijiechen reviewed Mar 21, 2025

View reviewed changes

pkg/plugins/runtime/k8s/controllers/pod_controller.go Outdated Show resolved Hide resolved

jijiechen reviewed Mar 21, 2025

View reviewed changes

feat(inbounds): remove unneeded log

Loading
Loading status checks…

35f8d6d

Signed-off-by: Marcin Skalski <[email protected]>

lobkovilya reviewed Mar 24, 2025

View reviewed changes

lukidzi assigned Automaat Mar 24, 2025

feat(kuma-cp): add information about new rules api and dataplane kind…

Loading
Loading status checks…

05ba8ae

… in upgrade md Signed-off-by: Marcin Skalski <[email protected]>

slonka requested a review from lobkovilya March 28, 2025 10:15

lobkovilya reviewed Mar 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(kuma-cp): deduplicate dataplane inbounds by address and port combination #12760

feat(kuma-cp): deduplicate dataplane inbounds by address and port combination #12760

Automaat commented Feb 5, 2025 •

edited

Loading

github-actions bot commented Feb 5, 2025

jijiechen Feb 24, 2025

jijiechen left a comment

jijiechen Mar 21, 2025

Automaat Mar 21, 2025

jijiechen Mar 21, 2025

Automaat Mar 21, 2025

lobkovilya Mar 24, 2025

Automaat Mar 25, 2025

lobkovilya Mar 24, 2025

Automaat Mar 25, 2025

lobkovilya Mar 24, 2025

Automaat Mar 25, 2025

lobkovilya Mar 24, 2025

Automaat Mar 25, 2025

lobkovilya Mar 24, 2025

Automaat Mar 25, 2025

lobkovilya Mar 24, 2025

Automaat Mar 25, 2025

lobkovilya Mar 28, 2025

jijiechen Apr 1, 2025

lobkovilya Mar 28, 2025

feat(kuma-cp): deduplicate dataplane inbounds by address and port combination #12760

Are you sure you want to change the base?

feat(kuma-cp): deduplicate dataplane inbounds by address and port combination #12760

Conversation

Automaat commented Feb 5, 2025 • edited Loading

Motivation

github-actions bot commented Feb 5, 2025

Reviewer Checklist

Choose a reason for hiding this comment

jijiechen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Automaat commented Feb 5, 2025 •

edited

Loading